Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Port with NodePort for creating LB members #12262

Merged
merged 1 commit into from
Aug 18, 2015
Merged

Replace Port with NodePort for creating LB members #12262

merged 1 commit into from
Aug 18, 2015

Conversation

jpgxs
Copy link
Contributor

@jpgxs jpgxs commented Aug 5, 2015

Fixes issue #12261

Each member should be created using the NodePort value or the load balancer will map to the wrong port on the nodes.

@k8s-bot
Copy link

k8s-bot commented Aug 5, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@justinsb
Copy link
Member

justinsb commented Aug 5, 2015

So the GCE load balancer preserves the target IP, so we rely on that source IP to identify load-balanced traffic. The AWS load balancer (ELB) does not preserve the target IP, so we have to use NodePort.

How does load-balanced traffic on OpenStack present itself? Is the target IP once it hits the node the external IP of the load-balancer, or is it the IP of the node? And which OpenStack setup are you running here - I think I would enjoy bringing up a commonly-used OpenStack configuration and giving Kubernetes a try there!

Your patch looks right, but I want to be sure that e.g. we're not breaking another OpenStack configuration here.

I think @anguslees wrote the original code so may be able to comment here also.

@jpgxs
Copy link
Contributor Author

jpgxs commented Aug 5, 2015

Ah I see; I'm running Kubernetes on CoreOS with flannel (on OpenStack, of course).

It's hard to tell if the target is preserved as the members fail to actually initialize - they fail to get a connection and will hang in an Initializing state.

There's also no configuration for the real target IP so health monitors (e.g. TCP) will also fail.

@anguslees
Copy link
Member

Huh, it seems k8s networking has changed quite a bit since I wrote this code...
Yes, this looks like the correct change from the k8s NodePort docs I've just read.

@justinsb: OpenStack loadbalancers are mostly like AWS's. The destination IP on the packet that gets sent to the LB backend will be the IP of that node (aka LB pool member - the addr+port given in the Create() call being modified in this change).

@mbforbes
Copy link
Contributor

needs rebase and an owner; @brendandburns as temp

edit: merge → rebase

@mbforbes mbforbes added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2015
@jpgxs
Copy link
Contributor Author

jpgxs commented Aug 17, 2015

Rebased

@brendandburns brendandburns added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2015
@brendandburns
Copy link
Contributor

Blind LGTM, since I don't really know OpenStack...

roberthbailey added a commit that referenced this pull request Aug 18, 2015
Replace Port with NodePort for creating LB members
@roberthbailey roberthbailey merged commit ca168ce into kubernetes:master Aug 18, 2015
dcbw added a commit to dcbw/kubernetes that referenced this pull request Feb 12, 2016
Reliably reproducible on two up-to-date Fedora 23 machines using
go 1.5.3, both one Core i7-4770R and a Core i7-4790.

golang/go#12262
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Replace Port with NodePort for creating LB members
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants